feat: Expose new kafka receiver/exporter knobs after upgrade to v0.158.0 - #6868
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
💻 Deploy preview deleted (feat: Expose new kafka receiver/exporter knobs after upgrade to v0.158.0). |
clayton-cornell
added
the
type/docs
There was a problem hiding this comment.
AI noted that introducing max_broker_write_bytes without exposing it in the converter could break previously working configs like,
producer:
max_message_bytes: 209715200
max_broker_write_bytes: 209715200
which would have ignored the field previously but will now error because max_broker_write_bytes didn't come along. It also found a second converter issues that came with the bump since upstream no longer defaults a group_rebalance_strategy an exist config which did not set it will fail to load.
I don't know that the converters are a big deal but might be good to ensure it's not going to break an existing config.
| // MaxBrokerWriteBytes is the maximum bytes the producer will write to a broker | ||
| // in a single request. Must be greater than or equal to max_message_bytes, and | ||
| // at least 100 MiB |
There was a problem hiding this comment.
The wording on here is a bit awkward, checking upstream I think it's suggesting this value needs to be changed when max_message_bytes goes above 100MiB which is the default value here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/kafkaexporter/README.md
There was a problem hiding this comment.
hmm, we could just simplify it to this maybe
the maximum bytes the producer will write to a broker in a single request. Must be greater than or equal to max_message_bytes
there is a minimum value for max_broker_write_bytes of 100MiB though - it's enforced upstream but not clearly documented
There was a problem hiding this comment.
Ugh how annoying, from a purely practical standpoint maybe we mirror the upstream documentation for now? Edit: It;s a code comment that's accurate for the code so probably fine.
…ka.md Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
…ies is handled correctly
|
@kgeckhart yeah that's true,
banged my head over this a bit, upstream never set a default (v0.153.0 and v0.158.0). I think this was already broken, I tested it on main - If you pass through otel config currently and omit Alloy sets a default to I think then if we get otel config which omits |
| MaxFetchSize: 1048576, | ||
| MaxPartitionFetchSize: 1048576, | ||
| MaxFetchWait: 250 * time.Millisecond, | ||
| GroupRebalanceStrategy: "range", |
There was a problem hiding this comment.
this default setting was moved to below so that we can ensure it's only set if GroupRebalanceStrategies isn't set
3 participants
Brief description of Pull Request
A follow up to the main upgrade: #6842
This introduces some further knobs that are available after the upgrade from contrib v0.153.0 to v0.158.0
Pull Request Details
otelcol.kafka.receiver => exposes the
group_rebalance_strategieswhich is intended to be used instead of the now deprecategroup_rebalance_strategy- the deprecated value is still valid so this is not a breaking changeotelcol.kafka.exporter => exposes
max_broker_write_bytesandlinger.max_broker_write_bytesmust be greater than or equal tomax_message_byteswhich we hardcode to 1000000. This condition is a breaking change from upstream that is documented in the changelog for the upgradeNotes to the Reviewer
This PR also includes a fix to our OTel dashboards, addressing a breaking change upstream that renames the
memory_limiterprocessor internal metricsPR Checklist